Feat/testing implementation #2212
Conversation
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds local Hardhat support and developer workflows: local network detection, conditional deployments (token and timing), new local token contract, async wagmi artifact merging, Hardhat RPC routing in the web app, commit/reveal flow refactor to use centralized commit builders and executeCommit, plus README and Blockscout local instructions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Commit Component
participant Hook as useCastCommit
participant Exec as executeCommit / buildCommitTxn
participant Wallet as walletClient
participant Chain as Blockchain
User->>UI: click "Commit" (choice, justification)
UI->>Hook: call castCommit(type, disputeId, choice, ...)
Hook->>Exec: buildCommitTxn(params, context)
Exec->>Wallet: walletClient.writeContract(tx)
Wallet->>Chain: submit transaction
Chain-->>Wallet: tx mined
Wallet-->>Exec: tx result
Exec-->>Hook: return status
Hook-->>UI: update UI / invalidate queries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @contracts/deploy/utils/deployTokens.ts:
- Around line 24-25: Fix the typo in the inline comment inside deployTokens.ts:
change "ERC20contract" to "ERC20 contract" in the comment that explains swapping
the local ERC20 implementation to one with increaseAllowance (refer to the
comment containing "increaseAllowance" in deployTokens.ts).
In @contracts/src/token/mock/PinakionV2Local.sol:
- Around line 25-34: The recoverTokens function incorrectly wraps
SafeERC20.safeTransfer (which returns void) in a require and uses .send for ETH;
remove the require and call the safeTransfer library call directly (e.g.,
token.safeTransfer(owner(), balance) or SafeERC20.safeTransfer(token, owner(),
balance) depending on imports), and replace
payable(owner()).send(address(this).balance) with a call pattern that bubbles
errors (e.g., (bool success, ) = payable(owner()).call{value:
address(this).balance}(""); require(success, "ETH transfer failed");) so
failures revert correctly; keep references to recoverTokens, token,
safeTransfer, and owner() when making the edits.
In @README.md:
- Around line 223-239: Fix the malformed link fragment and heading level:
replace the fragment text "#####shell-1---local-rpc-with-contracts-deployed"
used in the [hardhat node] link with a proper fragment starting with a single
hash (e.g., "#shell-1---local-rpc-with-contracts-deployed"), and change the
"#### Step 2: Start the Docker compose stack" heading to "### Step 2: Start the
Docker compose stack" so the heading hierarchy flows from "##" to "###"; also
ensure the localhost URL is written as <http://localhost> for consistency.
In @web/src/consts/processEnvConsts.ts:
- Line 23: The file exports an unused legacy function isLocalDeployment that
reads process.env.REACT_APP_DEPLOYMENT and duplicates behavior already
implemented in index.ts; either delete web/src/consts/processEnvConsts.ts
entirely, or move/merge the isLocalDeployment export into
web/src/consts/index.ts replacing process.env.REACT_APP_DEPLOYMENT with
import.meta.env.REACT_APP_DEPLOYMENT to match Vite usage, and remove any
duplicate exports so only the index.ts implementation remains.
🧹 Nitpick comments (5)
contracts/src/token/mock/PinakionV2Local.sol (1)
36-51: Add a comment explaining whyincreaseAllowanceanddecreaseAllowanceare manually implemented.These functions were removed from OpenZeppelin ERC20 in v5 as non-standard helpers, but your contract reimplements them. Adding a comment clarifying the rationale (compatibility with existing code, backwards compatibility, etc.) will help future maintainers understand the intent.
contracts/deploy/utils/index.ts (1)
31-32: Consider clarifying the comment.The comment mentions the network name is "hardhat" when deployed while starting the node, but the code checks for
network.name === "localhost". While thechainIdcheck correctly handles both cases (covering "hardhat" name via the chainId), the comment could be clearer.🔎 Suggested comment improvement
-// when deployed while starting node, the network name is "hardhat", the common factor for determining local node is chainId +// The network name can be "localhost" or "hardhat" (when deployed while starting the node). +// ChainId 31337 is the common factor for reliably detecting local Hardhat nodes. export const isLocalhost = (network: Network) => network.name === "localhost" || network.config.chainId === 31337;contracts/deploy/utils/deployTokens.ts (1)
26-27: Consider extracting the repeated condition.The condition
ticker === "PNK" && isLocalhost(hre.network)is repeated on consecutive lines. While this works correctly, extracting it to a variable would improve maintainability.🔎 Suggested refactor
- const contractName = ticker === "PNK" && isLocalhost(hre.network) ? "PinakionV2Local" : "TestERC20"; - const args = ticker === "PNK" && isLocalhost(hre.network) ? [] : [ticker, ticker]; + const isPNKLocalDeployment = ticker === "PNK" && isLocalhost(hre.network); + const contractName = isPNKLocalDeployment ? "PinakionV2Local" : "TestERC20"; + const args = isPNKLocalDeployment ? [] : [ticker, ticker];web/src/context/Web3Provider.tsx (1)
53-56: Potential undefined access indefaultTransportwebsocket fallback.
chain.rpcUrls.default?.webSocket?.[0]may beundefinedfor chains that don't provide a default WebSocket URL. WhilewebSocket(undefined)is handled gracefully by viem (it becomes a no-op in the fallback), consider making the fallback more explicit or adding a comment explaining this behavior.🔎 Optional: Make undefined handling explicit
const defaultTransport = (chain: AppKitNetwork) => - fallback([http(chain.rpcUrls.default?.http?.[0]), webSocket(chain.rpcUrls.default?.webSocket?.[0])]); + fallback([ + http(chain.rpcUrls.default?.http?.[0]), + // WebSocket may not be available for all chains; fallback handles undefined gracefully + ...(chain.rpcUrls.default?.webSocket?.[0] ? [webSocket(chain.rpcUrls.default.webSocket[0])] : []), + ]);contracts/wagmi.config.hardhat.ts (1)
2-2: Same import assertion syntax note asweb/wagmi.config.ts.Biome flags the
assert { type: "json" }syntax. Consider updating towith { type: "json" }for consistency if your TypeScript/bundler versions support import attributes.🔎 Proposed update to import attributes syntax
-import IHomeGateway from "./artifacts/src/gateway/interfaces/IHomeGateway.sol/IHomeGateway.json" assert { type: "json" }; +import IHomeGateway from "./artifacts/src/gateway/interfaces/IHomeGateway.sol/IHomeGateway.json" with { type: "json" };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
README.mdcontracts/deploy/00-home-chain-arbitration.tscontracts/deploy/00-rng-chainlink.tscontracts/deploy/utils/deployTokens.tscontracts/deploy/utils/index.tscontracts/deployments/hardhat.viem.tscontracts/hardhat.config.tscontracts/package.jsoncontracts/src/token/mock/PinakionV2Local.solcontracts/wagmi.config.hardhat.tsservices/graph-node/docker-compose.ymlsubgraph/scripts/all.shweb/.env.local.publicweb/src/consts/chains.tsweb/src/consts/index.tsweb/src/consts/processEnvConsts.tsweb/src/context/Web3Provider.tsxweb/src/utils/getGraphqlUrl.tsweb/wagmi.config.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
contracts/deploy/00-home-chain-arbitration.tscontracts/deploy/utils/index.tsweb/src/consts/processEnvConsts.tsweb/src/consts/index.tscontracts/deploy/00-rng-chainlink.ts
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/utils/getGraphqlUrl.ts
📚 Learning: 2024-10-15T16:18:32.543Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1687
File: web/src/context/AtlasProvider.tsx:225-244
Timestamp: 2024-10-15T16:18:32.543Z
Learning: In `web/src/context/AtlasProvider.tsx`, the `atlasUri` variable comes from environment variables and does not change, so it does not need to be included in dependency arrays.
Applied to files:
web/.env.local.publicweb/src/consts/index.ts
📚 Learning: 2024-10-22T09:38:20.093Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:1-1
Timestamp: 2024-10-22T09:38:20.093Z
Learning: In the TypeScript file `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `Abi` type is parsed later in the action functions, so importing `Abi` from `viem` in this file is unnecessary.
Applied to files:
web/wagmi.config.ts
📚 Learning: 2025-09-03T22:48:32.972Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Applied to files:
contracts/src/token/mock/PinakionV2Local.sol
🧬 Code graph analysis (6)
contracts/deploy/00-home-chain-arbitration.ts (1)
contracts/deploy/utils/index.ts (2)
isDevnet(30-30)isLocalhost(32-32)
web/src/consts/processEnvConsts.ts (1)
web/src/consts/index.ts (1)
isLocalDeployment(33-33)
web/src/consts/index.ts (1)
web/src/consts/processEnvConsts.ts (1)
isLocalDeployment(23-23)
contracts/deploy/00-rng-chainlink.ts (1)
contracts/deploy/utils/index.ts (1)
isLocalhost(32-32)
web/src/context/Web3Provider.tsx (3)
contracts/deploy/utils/index.ts (1)
isLocalhost(32-32)web/src/consts/index.ts (2)
isLocalDeployment(33-33)HARDHAT_NODE_RPC(53-53)web/src/consts/processEnvConsts.ts (1)
isLocalDeployment(23-23)
web/src/consts/chains.ts (2)
web/src/consts/processEnvConsts.ts (3)
DEFAULT_CHAIN(32-32)isLocalDeployment(23-23)isProductionDeployment(22-22)web/src/consts/index.ts (2)
isLocalDeployment(33-33)isProductionDeployment(31-31)
🪛 Biome (2.1.2)
web/wagmi.config.ts
[error] 12-12: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 12-12: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
contracts/wagmi.config.hardhat.ts
[error] 2-2: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 2-2: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
🪛 dotenv-linter (4.0.0)
web/.env.local.public
[warning] 3-3: [UnorderedKey] The REACT_APP_CORE_SUBGRAPH key should go before the REACT_APP_DEPLOYMENT key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
README.md
113-113: Dollar signs used before commands without showing output
(MD014, commands-show-output)
225-225: Link fragments should be valid
(MD051, link-fragments)
227-227: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
239-239: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Mend Security Check
🔇 Additional comments (27)
contracts/hardhat.config.ts (1)
60-63: LGTM!The mining configuration with
auto: trueand a 5-second interval provides predictable block times for local development and testing. This is a standard pattern for simulating realistic block production while maintaining fast feedback loops.services/graph-node/docker-compose.yml (2)
26-26: Acknowledge local-only configuration for non-deterministic fulltext search.Enabling
GRAPH_ALLOW_NON_DETERMINISTIC_FULLTEXT_SEARCHis appropriate for local development but should never be used in production as it can cause inconsistent query results across Graph Node instances. This is acceptable since this docker-compose is for local development only.
38-38: LGTM!The postgres command reformatting is a non-functional change that improves readability.
contracts/package.json (2)
63-65: LGTM!Adding the
Resolvertag to bothstart-localanddeploy-localscripts ensures consistent local deployment of resolver contracts alongside arbitration components.
70-70: LGTM!The
populate:localscript provides a convenient way to seed local development with courts and policy registry data fromv2_devnet, streamlining the local setup workflow.web/.env.local.public (1)
2-5: LGTM!The environment variable updates correctly configure the web app for local development, pointing to local Graph Node endpoints and setting the deployment mode to
localhost. This aligns with the broader local development infrastructure changes in this PR.subgraph/scripts/all.sh (1)
13-13: The removal ofcore-universityfrom the batch loop is intentional and safe.The
core-universitysubgraph remains fully functional with dedicated npm scripts inpackage.json(lines 24-32), including update, codegen, build, test, deploy, and rebuild commands. The change toall.shis a deliberate scope reduction for the batch automation script, allowing developers to runcoreanddrtsubgraphs together while invokingcore-universitycommands individually when needed. This does not break any workflows.contracts/deploy/00-rng-chainlink.ts (2)
3-3: LGTM!The addition of
isLocalhostto the import aligns with the local development support introduced across the deployment scripts.
79-79: LGTM!The conditional timeout logic is well-designed: 10 seconds for local development (appropriate for the 5-second Hardhat mining interval) and 30 minutes for production networks.
web/src/utils/getGraphqlUrl.ts (2)
1-1: LGTM!The addition of
hardhatto the wagmi/chains import enables local development support.
13-14: LGTM!The hardhat chain mapping follows the same pattern as other chains, using an environment variable with an appropriate fallback message.
contracts/deploy/00-home-chain-arbitration.ts (2)
6-6: LGTM!The addition of
isLocalhostto the imports enables localhost detection for devnet-equivalent behavior.
50-52: LGTM!Treating localhost as devnet-equivalent for timing constants is appropriate. The shorter
minStakingTime(180s vs 1800s) andmaxFreezingTime(600s vs 1800s) enable faster testing cycles during local development.contracts/deploy/utils/deployTokens.ts (2)
4-4: LGTM!The addition of
isLocalhostto the imports follows the existing pattern.
28-33: LGTM!The conditional contract deployment correctly uses
PinakionV2Localfor PNK on localhost andTestERC20otherwise, with appropriate constructor arguments for each case.web/src/consts/chains.ts (2)
14-18: LGTM - Local deployment chain selection is correctly prioritized.The ternary chain correctly checks
isLocalDeployment()first, then falls back to the production/testnet logic. This ensures local development uses Hardhat while preserving existing behavior for other environments.
21-27: The code correctly handles the chain/transport mismatch for local deployments. WhileQUERY_CHAINSincludes gnosis/gnosisChiado and mainnet regardless of deployment type, thecreateAppKitconfiguration usesallowUnsupportedChain: true(line 110 of Web3Provider.tsx) which gracefully allows chains without corresponding transports. Additionally, mainnet is included in the localhost transport for ENS resolution. Since gnosis/gnosisChiado are read-only query chains not needed during local development, no changes are required.web/src/consts/index.ts (2)
52-53: LGTM - Standard Hardhat RPC endpoint.The
HARDHAT_NODE_RPCconstant uses the default Hardhat network address. This is the standard endpoint for local Hardhat nodes.
33-33: TheisLocalDeploymentfunction inindex.tsis the only active version—processEnvConsts.tsis not imported anywhere in the codebase.The concern about duplication is partially valid, but
processEnvConsts.ts(usingprocess.env) appears to be unused or legacy code. Only theimport.meta.envversion inindex.tsis actively imported and used bychains.tsandWeb3Provider.tsx. Consider clarifying whetherprocessEnvConsts.tsshould be maintained or removed.Likely an incorrect or invalid review comment.
web/src/context/Web3Provider.tsx (2)
67-71: LGTM - Local transport correctly includes mainnet for ENS resolution.The local transport configuration properly includes both the Hardhat node and mainnet (via Alchemy) to support ENS resolution during local development.
98-108: LGTM - SDK and AppKit configuration consistently updated for local deployment.Both
configureSDKandcreateAppKituse the sameisLocalhostcheck to select the appropriate chain and transport, maintaining consistency across the Web3 stack.README.md (1)
110-123: LGTM - Clear instructions for local data population and artifact generation.The new section clearly documents the required steps after local deployment, including the court population and wagmi/viem bindings regeneration.
web/wagmi.config.ts (3)
92-96: LGTM - Localhost deployment case correctly configured.The new
localhostcase properly setsviemNetworkto"hardhat",hardhatNetworkto"localhost", and useshardhatViemfor the arbitrator contracts, maintaining consistency with the artifact directory structure.
20-20: Good design: OptionalhardhatChainNameparameter enables flexible artifact resolution.The
readArtifactsfunction now accepts an optionalhardhatChainNameto decouple the viem chain name from the Hardhat deployment directory name. This elegantly handles cases likelocalhost/hardhatmapping.Also applies to: 51-51
12-16: Theassert { type: "json" }syntax is the standard ES2022 import assertions syntax and is fully supported by TypeScript 5.6.3 with module set to ESNext. This same syntax is used consistently across 7 files in the project (web/wagmi.config.ts, web-devtools/wagmi.config.ts, web/scripts/gitInfo.js, and multiple wagmi config files). While TypeScript 5.5+ nominally preferswith { type: "json" }for forward compatibility,assertremains valid and functional. The suggestion to update only this one file creates an inconsistency with the rest of the codebase and is not a functional requirement.contracts/wagmi.config.hardhat.ts (2)
5-27: LGTM - Async config with multi-network artifact discovery.The async
getConfigfunction properly:
- Reads artifacts from localhost, gnosisChiado, and sepolia networks
- Logs discovered artifacts for debugging visibility
- Merges artifacts across networks using the helper
- Includes
IHomeGatewayin the final outputThe comment on line 10 about network renaming breaking other scripts is a useful note for future maintainers.
3-3: No action required. ThewagmiHelpersmodule exists atcontracts/scripts/wagmiHelpers.tsand properly exports all three required functions:getAbi(line 11),readArtifacts(line 15), andmerge(line 45).
| // locally the ERC20contract lacks `increaseAllowance` function, | ||
| // so we swap it with an updated contract to allow local development |
There was a problem hiding this comment.
Fix typo in comment.
The comment contains a typo: "ERC20contract" should be "ERC20 contract".
🔎 Proposed fix
- // locally the ERC20contract lacks `increaseAllowance` function,
+ // locally the ERC20 contract lacks `increaseAllowance` function,
// so we swap it with an updated contract to allow local development📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // locally the ERC20contract lacks `increaseAllowance` function, | |
| // so we swap it with an updated contract to allow local development | |
| // locally the ERC20 contract lacks `increaseAllowance` function, | |
| // so we swap it with an updated contract to allow local development |
🤖 Prompt for AI Agents
In @contracts/deploy/utils/deployTokens.ts around lines 24 - 25, Fix the typo in
the inline comment inside deployTokens.ts: change "ERC20contract" to "ERC20
contract" in the comment that explains swapping the local ERC20 implementation
to one with increaseAllowance (refer to the comment containing
"increaseAllowance" in deployTokens.ts).
| function recoverTokens(address _token) public onlyOwner { | ||
| if (_token == address(0)) { | ||
| require(payable(owner()).send(address(this).balance), "Transfer failed"); | ||
| return; | ||
| } | ||
|
|
||
| IERC20 token = IERC20(_token); | ||
| uint balance = token.balanceOf(address(this)); | ||
| require(token.safeTransfer(payable(owner()), balance), "Token transfer failed"); | ||
| } |
There was a problem hiding this comment.
Critical: safeTransfer returns void, not bool—this will always revert.
SafeERC20.safeTransfer does not return a boolean; it reverts on failure. The require statement on line 33 will always fail because the return value is void (treated as false).
Additionally, using .send() on line 27 is risky as it only forwards 2300 gas, which may fail for contracts with complex receive/fallback functions.
🔎 Proposed fix
function recoverTokens(address _token) public onlyOwner {
if (_token == address(0)) {
- require(payable(owner()).send(address(this).balance), "Transfer failed");
+ (bool success, ) = payable(owner()).call{value: address(this).balance}("");
+ require(success, "Transfer failed");
return;
}
IERC20 token = IERC20(_token);
uint balance = token.balanceOf(address(this));
- require(token.safeTransfer(payable(owner()), balance), "Token transfer failed");
+ token.safeTransfer(owner(), balance);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function recoverTokens(address _token) public onlyOwner { | |
| if (_token == address(0)) { | |
| require(payable(owner()).send(address(this).balance), "Transfer failed"); | |
| return; | |
| } | |
| IERC20 token = IERC20(_token); | |
| uint balance = token.balanceOf(address(this)); | |
| require(token.safeTransfer(payable(owner()), balance), "Token transfer failed"); | |
| } | |
| function recoverTokens(address _token) public onlyOwner { | |
| if (_token == address(0)) { | |
| (bool success, ) = payable(owner()).call{value: address(this).balance}(""); | |
| require(success, "Transfer failed"); | |
| return; | |
| } | |
| IERC20 token = IERC20(_token); | |
| uint balance = token.balanceOf(address(this)); | |
| token.safeTransfer(owner(), balance); | |
| } |
🤖 Prompt for AI Agents
In @contracts/src/token/mock/PinakionV2Local.sol around lines 25 - 34, The
recoverTokens function incorrectly wraps SafeERC20.safeTransfer (which returns
void) in a require and uses .send for ETH; remove the require and call the
safeTransfer library call directly (e.g., token.safeTransfer(owner(), balance)
or SafeERC20.safeTransfer(token, owner(), balance) depending on imports), and
replace payable(owner()).send(address(this).balance) with a call pattern that
bubbles errors (e.g., (bool success, ) = payable(owner()).call{value:
address(this).balance}(""); require(success, "ETH transfer failed");) so
failures revert correctly; keep references to recoverTokens, token,
safeTransfer, and owner() when making the edits.
| ## Run a local Blockscout explorer to inspect transactions on hardhat (Optional) | ||
|
|
||
| Make sure [hardhat node](#####shell-1---local-rpc-with-contracts-deployed) is running. | ||
|
|
||
| #### Step 1 - Clone [blockscout](https://github.com/blockscout/blockscout/blob/master/docker-compose/README.md) repo | ||
|
|
||
| ```bash | ||
| git clone https://github.com/blockscout/blockscout.git | ||
| ``` | ||
|
|
||
| #### Step 2: Start the Docker compose stack | ||
|
|
||
| ```bash | ||
| docker-compose -f hardhat-network.yml up -d | ||
| ``` | ||
|
|
||
| This should run a Blockscout locally at http://localhost. |
There was a problem hiding this comment.
Fix heading hierarchy and link fragment.
Static analysis flagged two issues:
- Line 225: The link fragment
#####shell-1---local-rpc-with-contracts-deployedappears malformed (uses 5 hashes instead of matching the actual heading format) - Line 227: Heading jumps from
##(h2) directly to####(h4), skipping h3
🔎 Proposed fixes
## Run a local Blockscout explorer to inspect transactions on hardhat (Optional)
-Make sure [hardhat node](#####shell-1---local-rpc-with-contracts-deployed) is running.
+Make sure [hardhat node](#shell-1---local-rpc-with-contracts-deployed) is running.
-#### Step 1 - Clone [blockscout](https://github.com/blockscout/blockscout/blob/master/docker-compose/README.md) repo
+### Step 1 - Clone [blockscout](https://github.com/blockscout/blockscout/blob/master/docker-compose/README.md) repo
```bash
git clone https://github.com/blockscout/blockscout.git-#### Step 2: Start the Docker compose stack
+### Step 2: Start the Docker compose stack
docker-compose -f hardhat-network.yml up -d-This should run a Blockscout locally at http://localhost.
+This should run a Blockscout locally at http://localhost.
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
## Run a local Blockscout explorer to inspect transactions on hardhat (Optional)
Make sure [hardhat node](#shell-1---local-rpc-with-contracts-deployed) is running.
### Step 1 - Clone [blockscout](https://github.com/blockscout/blockscout/blob/master/docker-compose/README.md) repo
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
225-225: Link fragments should be valid
(MD051, link-fragments)
227-227: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
239-239: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In @README.md around lines 223 - 239, Fix the malformed link fragment and
heading level: replace the fragment text
"#####shell-1---local-rpc-with-contracts-deployed" used in the [hardhat node]
link with a proper fragment starting with a single hash (e.g.,
"#shell-1---local-rpc-with-contracts-deployed"), and change the "#### Step 2:
Start the Docker compose stack" heading to "### Step 2: Start the Docker compose
stack" so the heading hierarchy flows from "##" to "###"; also ensure the
localhost URL is written as <http://localhost> for consistency.
| export const ETH_SIGNATURE_REGEX = /^0x([a-fA-F0-9]{130})+$|^0x$/; | ||
|
|
||
| export const isProductionDeployment = () => process.env.REACT_APP_DEPLOYMENT === "mainnet"; | ||
| export const isLocalDeployment = () => process.env.REACT_APP_DEPLOYMENT === "localhost"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the exact code at the specified locations
echo "=== processEnvConsts.ts (around line 23) ==="
head -30 web/src/consts/processEnvConsts.ts | cat -n
echo -e "\n=== index.ts (around line 32) ==="
head -40 web/src/consts/index.ts | cat -nRepository: kleros/kleros-v2
Length of output: 4064
🏁 Script executed:
#!/bin/bash
# Check for vite configuration
echo "=== Vite config files ==="
find . -maxdepth 2 -name "vite.config.*" -o -name "vite.config.ts" -o -name "vite.config.js"
echo -e "\n=== package.json dev dependencies (checking for vite) ==="
cat package.json | jq '.devDependencies | keys[] | select(contains("vite"))'Repository: kleros/kleros-v2
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Analyze the full distribution of env variable patterns
echo "=== process.env usage in web/src ==="
rg -c "process\.env\." web/src --type=ts --type=tsx -g '!*.test.*' | wc -l
rg "process\.env\." web/src --type=ts --type=tsx -g '!*.test.*' | head -15
echo -e "\n=== import.meta.env usage in web/src ==="
rg -c "import\.meta\.env\." web/src --type=ts --type=tsx -g '!*.test.*' | wc -l
rg "import\.meta\.env\." web/src --type=ts --type=tsx -g '!*.test.*' | head -15Repository: kleros/kleros-v2
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Check imports of both files to see which is actively used
echo "=== Imports of processEnvConsts.ts ==="
rg "from.*processEnvConsts" web/src -A 2 -B 2
echo -e "\n=== Imports of consts/index.ts ==="
rg 'from.*["\']consts["\']' web/src -A 2 -B 2 | head -40
echo -e "\n=== Check for explicit index imports ==="
rg 'from.*consts/index' web/srcRepository: kleros/kleros-v2
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about these files
echo "=== File headers/comments in processEnvConsts.ts ==="
head -5 web/src/consts/processEnvConsts.ts
echo -e "\n=== File headers/comments in index.ts ==="
head -5 web/src/consts/index.ts
echo -e "\n=== Check if processEnvConsts.ts is exported from index.ts ==="
rg "processEnvConsts" web/src/consts/Repository: kleros/kleros-v2
Length of output: 648
🏁 Script executed:
#!/bin/bash
# Check imports more carefully with proper escaping
echo "=== Imports of processEnvConsts.ts ==="
rg "processEnvConsts" web/src
echo -e "\n=== All imports from consts (index.ts) ==="
rg "from ['\"]consts['\"]" web/src | head -30Repository: kleros/kleros-v2
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Check tsconfig or any references to consts module resolution
echo "=== tsconfig.json references ==="
cat web/tsconfig.json | grep -A 5 -B 5 "paths\|baseUrl" || echo "No special path config found"
echo -e "\n=== Check web directory structure ==="
ls -la web/src/consts/Repository: kleros/kleros-v2
Length of output: 1172
🏁 Script executed:
#!/bin/bash
# Check if processEnvConsts is referenced from index.ts or exported
echo "=== Complete index.ts file ==="
cat web/src/consts/index.ts
echo -e "\n=== Check tsconfig paths more carefully ==="
cat web/tsconfig.json | jq '.compilerOptions.paths'Repository: kleros/kleros-v2
Length of output: 3163
🏁 Script executed:
#!/bin/bash
# Check if there are any direct imports of consts files in the entire web directory
echo "=== Direct imports from consts/ ==="
rg "from ['\"].*consts/" web/src | head -20
echo -e "\n=== Imports from consts without explicit file ==="
rg "from ['\"]consts['\"]" web/src | head -20Repository: kleros/kleros-v2
Length of output: 2045
🏁 Script executed:
#!/bin/bash
# Check git history of processEnvConsts to understand its purpose
echo "=== Check if processEnvConsts.ts is referenced in comments or README ==="
rg "processEnvConsts" web/ --type=md --type=txt
echo -e "\n=== Check all files in consts directory to see if any re-export from processEnvConsts ==="
grep -r "processEnvConsts" web/src/consts/
echo -e "\n=== See if there are any TypeScript declaration files that might reference it ==="
find web/src -name "*.d.ts" -exec grep -l "processEnvConsts" {} \;Repository: kleros/kleros-v2
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Double-check by looking for any dynamic imports or string references
echo "=== Search for any string references to processEnvConsts ==="
rg "processEnvConsts" web/
echo -e "\n=== Check if it's exported from barrel files ==="
grep -n "export.*from.*processEnvConsts" web/src/consts/*Repository: kleros/kleros-v2
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Final check: see if processEnvConsts.ts is actually being used in any build configs or if it's truly dead code
echo "=== Check vite.config.js for any special handling ==="
cat web/vite.config.js
echo -e "\n=== Check if there are any glob imports or dynamic requires ==="
rg "processEnvConsts|consts/\*" web/src --type=ts --type=tsxRepository: kleros/kleros-v2
Length of output: 1497
Remove or consolidate processEnvConsts.ts — it's unused dead code.
This file is not imported anywhere in the codebase and duplicates functionality already present in web/src/consts/index.ts. Additionally, it uses process.env.REACT_APP_DEPLOYMENT which is the legacy pattern; the active file (index.ts) correctly uses import.meta.env.REACT_APP_DEPLOYMENT for Vite. Either delete processEnvConsts.ts or consolidate the duplicate exports into index.ts.
🤖 Prompt for AI Agents
In @web/src/consts/processEnvConsts.ts at line 23, The file exports an unused
legacy function isLocalDeployment that reads process.env.REACT_APP_DEPLOYMENT
and duplicates behavior already implemented in index.ts; either delete
web/src/consts/processEnvConsts.ts entirely, or move/merge the isLocalDeployment
export into web/src/consts/index.ts replacing process.env.REACT_APP_DEPLOYMENT
with import.meta.env.REACT_APP_DEPLOYMENT to match Vite usage, and remove any
duplicate exports so only the index.ts implementation remains.
jaybuidl
added
Type: Toolchain ⚒️
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @web/src/actions/commit/helpers/index.ts:
- Around line 1-4: encodeShutterMessage currently uses choice.toString() but
relies on implicit conversion for salt; update the function
(encodeShutterMessage) to call salt.toString() as well when building the string
with SEPARATOR and justification to keep both bigint parameters handled
consistently.
In @web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx:
- Around line 37-44: The call to castCommit passes currentRoundIndex which can
be undefined (derived from disputeData?.dispute?.currentRoundIndex); guard it
before calling castCommit by returning early or showing an error/loading state
when currentRoundIndex is undefined, e.g., check if currentRoundIndex == null
and abort the commit flow (do not call castCommit) or provide a valid default,
then only call castCommit with the validated roundIndex and
setIsOpen(res.status) on success.
In @web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx:
- Around line 55-64: The call to castCommit uses currentRoundIndex which can be
undefined; before invoking castCommit in Commit.tsx check that currentRoundIndex
is a valid number (e.g., !== undefined/null) and bail out or show an error if
not set so you never pass undefined as roundIndex; update the code path that
currently calls castCommit({ ..., roundIndex: currentRoundIndex, ... }) to guard
on currentRoundIndex and only call castCommit when it is defined, referencing
the same symbols (castCommit, currentRoundIndex,
DisputeKits.GatedShutter/DisputeKits.Shutter, setIsOpen) so the UI handles the
missing round index safely.
🧹 Nitpick comments (9)
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
5-5: Unused import.
useLocalStorageis imported but no longer used after the refactor to directlocalStorage.getItem().-import { useLocalStorage } from "react-use";web/src/utils/crypto/generateSalt.ts (1)
15-16: Unnecessary optional chaining after validation.The optional chaining
signingAccount?.signMessageon line 15 is redundant sincesigningAccount.signMessageis already validated to exist on line 11. This is a minor inconsistency.♻️ Suggested cleanup
- const signature = await signingAccount?.signMessage({ message }); + const signature = await signingAccount.signMessage({ message });web/src/hooks/useCastCommit.tsx (1)
67-67: Hardcoded query key may become out of sync.The query key
["useDrawQuery"]is hardcoded. If the actual query definition uses a different key or includes additional parameters, this invalidation won't work as expected. Consider centralizing query keys to avoid mismatches.web/src/actions/commit/builders/index.ts (1)
14-23: Consider improving type safety in the builder registry.The
anytype in theRecordandas nevercast work correctly at runtime due to the discriminated union, but they bypass compile-time type checking. This is a common pattern for dispatch tables, but worth noting.If stricter typing is desired later, an overloaded function signature could enforce the relationship between
params.typeand the expected param type.web/src/actions/commit/builders/gated.builder.ts (1)
8-22: Consider validating chain support before address lookup.The address lookup
disputeKitGatedAddress[chain.id]will returnundefinedfor unsupported chains, which would cause a transaction failure. While the UI likely restricts to supported chains, a defensive check could provide a clearer error message.♻️ Optional defensive check
export const gatedCommitBuilder: CommitBuilder<GatedCommitParams, typeof disputeKitGatedAbi> = { build: async (params, context) => { const { disputeId, voteIds, choice, salt } = params; const { chain, account } = context; + const address = disputeKitGatedAddress[chain.id]; + if (!address) { + throw new Error(`DisputeKitGated not deployed on chain ${chain.id}`); + } + const commit = hashVote(choice, salt); return { account, - address: disputeKitGatedAddress[chain.id], + address, abi: disputeKitGatedAbi, functionName: "castCommit", args: [disputeId, voteIds, commit], chain, }; }, };web/src/actions/commit/builders/gatedShutter.builder.ts (1)
27-28: RedundantBigInt()conversion onsalt.According to
BaseCommitParamsinparams.ts,saltis already typed asbigint. TheBigInt(salt)calls are unnecessary and could be simplified.Suggested simplification
- const choiceCommit = hashVote(choice, BigInt(salt)); - const justificationCommit = hashJustification(BigInt(salt), justification); + const choiceCommit = hashVote(choice, salt); + const justificationCommit = hashJustification(salt, justification);web/src/actions/commit/builders/shutter.builder.ts (2)
13-38: Significant code duplication withgatedShutter.builder.ts.This builder shares nearly identical logic with
gatedShutter.builder.ts(env validation, message encoding, encryption, hashing). Consider extracting shared Shutter-specific logic into a helper function to reduce duplication.
27-28: RedundantBigInt()conversion onsalt.Same issue as
gatedShutter.builder.ts-saltis alreadybigintperBaseCommitParams.Suggested simplification
- const choiceCommit = hashVote(choice, BigInt(salt)); - const justificationCommit = hashJustification(BigInt(salt), justification); + const choiceCommit = hashVote(choice, salt); + const justificationCommit = hashJustification(salt, justification);web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
37-39: Duplicate data fetching for dispute details.The component receives
disputeas a prop (line 25) but also callsuseDisputeDetailsQuery(id)(line 37) just to getcurrentRoundIndex. Consider usingdispute?.currentRoundIndexfrom the prop instead to avoid redundant network requests.Suggested change
- const { data: disputeData } = useDisputeDetailsQuery(id); - - const currentRoundIndex = disputeData?.dispute?.currentRoundIndex; + const currentRoundIndex = dispute?.currentRoundIndex;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
web/package.jsonweb/src/actions/commit/builders/baseBuilder.tsweb/src/actions/commit/builders/classic.builder.tsweb/src/actions/commit/builders/gated.builder.tsweb/src/actions/commit/builders/gatedShutter.builder.tsweb/src/actions/commit/builders/index.tsweb/src/actions/commit/builders/shutter.builder.tsweb/src/actions/commit/context.tsweb/src/actions/commit/execute.tsweb/src/actions/commit/helpers/index.tsweb/src/actions/commit/params.tsweb/src/hooks/queries/useDrawQuery.tsweb/src/hooks/useCastCommit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/utils/crypto/generateSalt.tsweb/src/utils/crypto/hashJustification.tsweb/src/utils/crypto/hashVote.tsweb/src/utils/crypto/shutter.tsweb/tsconfig.json
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/hooks/queries/useDrawQuery.tsweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
Repo: kleros/kleros-v2 PR: 1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/hooks/queries/useDrawQuery.tsweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
web/src/actions/commit/execute.tsweb/src/actions/commit/builders/gated.builder.tsweb/src/hooks/useCastCommit.tsxweb/src/actions/commit/params.tsweb/src/actions/commit/context.tsweb/src/actions/commit/builders/classic.builder.tsweb/src/actions/commit/builders/shutter.builder.tsweb/src/actions/commit/builders/index.tsweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/actions/commit/params.tsweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2024-12-16T17:17:32.359Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1794
File: web/src/hooks/useStarredCases.tsx:13-18
Timestamp: 2024-12-16T17:17:32.359Z
Learning: In `useStarredCases.tsx`, when handling the `starredCases` Map from local storage, direct mutation is acceptable to prevent the overhead of copying, provided it doesn't adversely affect React's render cycle.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2024-10-09T10:22:41.474Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1582
File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90
Timestamp: 2024-10-09T10:22:41.474Z
Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsxweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2024-12-09T12:36:59.441Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx
📚 Learning: 2025-09-09T13:33:46.896Z
Learnt from: tractorss
Repo: kleros/kleros-v2 PR: 2117
File: web/src/components/DisputeFeatures/Features/GatedErc1155.tsx:51-66
Timestamp: 2025-09-09T13:33:46.896Z
Learning: The `setDisputeData` function in `NewDisputeContext` at `web/src/context/NewDisputeContext.tsx` has signature `(disputeData: IDisputeData) => void` and only accepts direct state values, not functional updates like standard React state setters. It cannot be used with the pattern `setDisputeData((prev) => ...)`.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/index.tsxweb/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
📚 Learning: 2025-12-17T14:55:22.988Z
Learnt from: kemuru
Repo: kleros/kleros-v2 PR: 1871
File: web/src/pages/Profile/Votes/index.tsx:69-75
Timestamp: 2025-12-17T14:55:22.988Z
Learning: In `web/src/pages/Profile/Votes/index.tsx`, fetching up to 1000 draws client-side is a temporary solution until the subgraph supports fetching `totalVotes` directly. This is acknowledged technical debt that will be addressed when the subgraph capability becomes available.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
Repo: kleros/kleros-v2 PR: 1632
File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42
Timestamp: 2024-06-27T10:11:54.861Z
Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
📚 Learning: 2025-05-09T13:39:15.086Z
Learnt from: tractorss
Repo: kleros/kleros-v2 PR: 1982
File: web/src/pages/Resolver/Parameters/NotablePersons/PersonFields.tsx:64-0
Timestamp: 2025-05-09T13:39:15.086Z
Learning: In PersonFields.tsx, the useEffect hook for address validation intentionally uses an empty dependency array to run only once on component mount. This is specifically designed for the dispute duplication flow when aliasesArray is already populated with addresses that need initial validation.
Applied to files:
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx
🧬 Code graph analysis (11)
web/src/actions/commit/execute.ts (3)
web/src/actions/commit/params.ts (1)
CommitParams(32-32)web/src/actions/commit/context.ts (1)
CommitContext(3-8)web/src/actions/commit/builders/index.ts (1)
buildCommitTxn(21-23)
web/src/actions/commit/builders/gated.builder.ts (3)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
GatedCommitParams(22-24)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)
web/src/hooks/useCastCommit.tsx (3)
web/src/actions/commit/params.ts (5)
ClassicCommitParams(12-14)ShutterCommitParams(16-20)GatedCommitParams(22-24)GatedShutterCommitParams(26-30)CommitParams(32-32)web/src/utils/crypto/generateSalt.ts (1)
generateSalt(10-19)web/src/actions/commit/execute.ts (1)
executeCommit(9-13)
web/src/actions/commit/params.ts (1)
web/src/consts/disputeFeature.ts (1)
DisputeKits(32-32)
web/src/actions/commit/builders/classic.builder.ts (3)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
ClassicCommitParams(12-14)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)
web/src/actions/commit/builders/shutter.builder.ts (6)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
ShutterCommitParams(16-20)web/src/actions/commit/helpers/index.ts (1)
encodeShutterMessage(2-4)web/src/utils/crypto/shutter.ts (1)
encrypt(151-175)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)web/src/utils/crypto/hashJustification.ts (1)
hashJustification(3-15)
web/src/actions/commit/builders/gatedShutter.builder.ts (6)
web/src/actions/commit/builders/baseBuilder.ts (1)
CommitBuilder(14-16)web/src/actions/commit/params.ts (1)
GatedShutterCommitParams(26-30)web/src/actions/commit/helpers/index.ts (1)
encodeShutterMessage(2-4)web/src/utils/crypto/shutter.ts (1)
encrypt(151-175)web/src/utils/crypto/hashVote.ts (1)
hashVote(11-23)web/src/utils/crypto/hashJustification.ts (1)
hashJustification(3-15)
web/src/actions/commit/builders/baseBuilder.ts (2)
web/src/actions/commit/params.ts (1)
CommitParams(32-32)web/src/actions/commit/context.ts (1)
CommitContext(3-8)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (4)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
useDisputeDetailsQuery(46-62)web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
getDeadline(145-156)web/src/hooks/useCastCommit.tsx (1)
useCastCommit(25-73)web/src/consts/disputeFeature.ts (1)
DisputeKits(32-32)
web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx (2)
web/src/hooks/queries/useDrawQuery.ts (1)
useDrawQuery(23-37)web/src/hooks/useVotingContext.tsx (1)
useVotingContext(134-134)
web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (3)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
useDisputeDetailsQuery(46-62)web/src/hooks/useCastCommit.tsx (1)
useCastCommit(25-73)web/src/consts/disputeFeature.ts (1)
DisputeKits(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
🔇 Additional comments (18)
web/package.json (1)
13-13: LGTM!The new
actionsalias is consistent with the existing alias pattern and aligns with the newly introducedsrc/actionsmodules.web/tsconfig.json (1)
15-17: LGTM!The
actions*path mapping correctly mirrors the package.json alias and follows the established pattern for other path aliases in this configuration.web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
70-76: LGTM on the localStorage change.Using
localStorage.getItem()directly during reveal is appropriate since you only need a one-time read of the stored salt/choice at execution time, rather than maintaining reactive state via the hook.web/src/hooks/queries/useDrawQuery.ts (1)
28-28: QueryKey is correctly structured for React Query cache management.The array format
["useDrawQuery", address, disputeID, roundID]is the proper approach, enabling React Query to properly compare and invalidate queries based on individual dependency parameters. The cache invalidation call inuseCastCommit.tsxcorrectly references the new key name, so no additional changes are needed.web/src/utils/crypto/hashVote.ts (1)
11-22: Consider edge case: empty string justification.The condition
justification === undefinedmeans an empty string""will be treated as a valid justification and hashed. If an empty justification should be treated the same as no justification, consider using a truthy check instead.If this is intentional (empty string is a valid justification distinct from none), then the implementation is correct.
web/src/utils/crypto/hashJustification.ts (1)
3-15: LGTM!The implementation correctly computes a deterministic justification hash using viem's cryptographic utilities. The ABI encoding structure is well-documented with inline comments.
web/src/actions/commit/context.ts (1)
3-8: LGTM!The
CommitContextinterface is well-structured and provides a clean abstraction for the commit workflow. The types from viem are appropriately used.web/src/hooks/useCastCommit.tsx (2)
25-72: Overall implementation looks good.The hook properly orchestrates the commit workflow with appropriate validation, salt generation, localStorage persistence, and query invalidation. The use of
useMutationfrom react-query is appropriate for this side-effect-heavy operation.
29-29: Missing validation forpublicClientbefore use.
publicClientis retrieved on line 29 but used with a non-null assertion on line 61 without prior validation. IfpublicClientis undefined (e.g., during initial render or network switching), this will cause a runtime error.🐛 Proposed fix
if (isUndefined(walletClient)) { throw new Error("WalletClient not defined. Is the wallet connected?"); } + + if (isUndefined(publicClient)) { + throw new Error("PublicClient not defined. Is the wallet connected?"); + }Then on line 61:
- const result = await wrapWithToast(executeTxn, publicClient!); + const result = await wrapWithToast(executeTxn, publicClient);Also applies to: 61-61
⛔ Skipped due to learnings
Learnt from: jaybuidl Repo: kleros/kleros-v2 PR: 2145 File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286 Timestamp: 2025-09-30T17:18:12.895Z Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.Learnt from: jaybuidl Repo: kleros/kleros-v2 PR: 1582 File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90 Timestamp: 2024-10-09T10:22:41.474Z Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1703 File: kleros-sdk/src/sdk.ts:1-3 Timestamp: 2024-10-22T10:23:15.789Z Learning: In `kleros-sdk/src/sdk.ts`, the `PublicClient` type is used and should not be flagged as unused.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1744 File: web/src/hooks/queries/useHomePageBlockQuery.ts:71-71 Timestamp: 2024-11-19T05:29:56.238Z Learning: In `web/src/hooks/queries/useHomePageBlockQuery.ts`, the non-null assertions on `blockNumber!` and `genesisBlock!` within `queryFn` are safe because `isEnabled` ensures that `queryFn` only runs when either `blockNumber` or `genesisBlock` is defined.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1703 File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61 Timestamp: 2024-10-14T13:58:25.708Z Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1703 File: kleros-sdk/src/sdk.ts:13-17 Timestamp: 2024-10-22T10:07:21.327Z Learning: In the SDK, `getPublicClient()` handles the scenario where `publicClient` is undefined by throwing `SdkNotConfiguredError`, so additional error handling in functions that call `getPublicClient()` is not necessary.web/src/actions/commit/execute.ts (1)
9-13: LGTM!Clean separation of transaction building and execution. The function correctly delegates to the centralized builder dispatch and executes via the wallet client. The return type (
Hash) is properly inferred fromwriteContract.web/src/actions/commit/builders/baseBuilder.ts (1)
1-16: LGTM - Well-designed builder interface.The generic interface cleanly constrains builder implementations while maintaining type safety with viem's
WriteContractParameters. This provides a solid foundation for the commit builder pattern.web/src/pages/Cases/CaseDetails/Voting/Classic/index.tsx (1)
25-31: LGTM - Simplified data flow with proper cache invalidation.The removal of
refetchfrom theuseDrawQuerydestructuring andCommitprops is a clean refactor. Query cache invalidation after commit is correctly handled in theuseCastCommitmutation'sonSuccesscallback viaqueryClient.invalidateQueries({ queryKey: ["useDrawQuery"] }), which is the idiomatic React Query pattern.web/src/actions/commit/builders/classic.builder.ts (1)
8-23: LGTM!Clean implementation following the builder pattern. The
hashVotecall correctly usessaltdirectly without redundant conversion.web/src/actions/commit/params.ts (2)
12-32: LGTM!Well-structured discriminated union types. The pattern enables type-safe dispatch to specific builders based on the
typefield.
1-10: No issues found.DisputeKitsis correctly defined as an enum insrc/consts/index.tswith the expected members (Classic, Shutter, Gated, GatedShutter), andparams.tsproperly imports and uses it as a type discriminator.web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (1)
26-47: LGTM on the refactored commit flow.The simplification using
useCastCommitis a good improvement over the previous multi-step wallet interaction. The dependency array is complete.web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
49-76: LGTM on callback structure.The decryption delay calculation with the 5-minute buffer is correctly implemented. The dependency array is complete.
web/src/actions/commit/builders/gatedShutter.builder.ts (1)
30-37: Add explicit chain support validation in all commit builders.The address lookups (
disputeKitGatedShutterAddress[chain.id], etc.) may returnundefinedif the chain is unsupported. While wagmi likely prevents unsupported chains from reaching this code, add an explicit guard for defensive programming, matching the pattern used inuseDisputeKitAddresses.ts:if (!(chain.id in disputeKitGatedShutterAddress)) { throw new Error(`Unsupported chain: ${chain.id}`); }This applies to all four builders:
classic.builder.ts,shutter.builder.ts,gated.builder.ts, andgatedShutter.builder.ts.⛔ Skipped due to learnings
Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1762 File: web/src/utils/parseWagmiError.ts:10-17 Timestamp: 2024-11-29T06:23:15.955Z Learning: In the `web/src/utils/parseWagmiError.ts` file and throughout the codebase, prefer using optional chaining to handle `undefined` or `null` values, including optional arrays, without adding explicit existence or length checks.Learnt from: Harman-singh-waraich Repo: kleros/kleros-v2 PR: 1703 File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61 Timestamp: 2024-10-14T13:58:25.708Z Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
| const SEPARATOR = "-"; | ||
| export const encodeShutterMessage = (choice: bigint, salt: bigint, justification: string) => { | ||
| return `${choice.toString()}${SEPARATOR}${salt}${SEPARATOR}${justification}`; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for decode functions, SEPARATOR usage, and related patterns
rg -n "SEPARATOR|decodeShutterMessage|split.*-" --type=ts web/src/Repository: kleros/kleros-v2
Length of output: 732
🏁 Script executed:
# Read the actual file to confirm its content
cat -n web/src/actions/commit/helpers/index.tsRepository: kleros/kleros-v2
Length of output: 288
🏁 Script executed:
# Look for related imports or usage of encodeShutterMessage
rg -n "encodeShutterMessage" --type=ts web/src/ -B 2 -A 5Repository: kleros/kleros-v2
Length of output: 3566
🏁 Script executed:
# Look for the encrypt function to see if it processes/decodes the message
rg -n "export.*encrypt" utils/crypto/shutter --type=ts -A 10Repository: kleros/kleros-v2
Length of output: 120
🏁 Script executed:
# Check for any validation on justification parameter
rg -n "justification" web/src/actions/commit/ --type=ts -B 2 -A 2Repository: kleros/kleros-v2
Length of output: 4330
Use explicit toString() for salt to match the choice parameter.
Line 3 uses choice.toString() explicitly but relies on implicit conversion for salt. For consistency, both bigint parameters should use explicit .toString():
- return `${choice.toString()}${SEPARATOR}${salt}${SEPARATOR}${justification}`;
+ return `${choice.toString()}${SEPARATOR}${salt.toString()}${SEPARATOR}${justification}`;🤖 Prompt for AI Agents
In @web/src/actions/commit/helpers/index.ts around lines 1 - 4,
encodeShutterMessage currently uses choice.toString() but relies on implicit
conversion for salt; update the function (encodeShutterMessage) to call
salt.toString() as well when building the string with SEPARATOR and
justification to keep both bigint parameters handled consistently.
| const res = await castCommit({ | ||
| type: isGated ? DisputeKits.Gated : DisputeKits.Classic, | ||
| disputeId: parsedDisputeID, | ||
| choice, | ||
| voteIds: parsedVoteIDs, | ||
| roundIndex: currentRoundIndex, | ||
| }); | ||
| if (walletClient && publicClient) { | ||
| await wrapWithToast(async () => await walletClient.writeContract(request), publicClient).then(({ status }) => { | ||
| setIsOpen(status); | ||
| }); | ||
| } | ||
| refetch(); | ||
| setIsOpen(res.status); |
There was a problem hiding this comment.
Potential undefined value for roundIndex.
currentRoundIndex (line 31) is derived from disputeData?.dispute?.currentRoundIndex and could be undefined if the query hasn't resolved. Passing undefined to castCommit may cause runtime issues in the builder or salt key generation.
Consider guarding against this:
Suggested guard
const handleCommit = useCallback(
async (choice: bigint) => {
+ if (currentRoundIndex === undefined) {
+ console.error("Round index not available");
+ return;
+ }
const res = await castCommit({
type: isGated ? DisputeKits.Gated : DisputeKits.Classic,
disputeId: parsedDisputeID,
choice,
voteIds: parsedVoteIDs,
roundIndex: currentRoundIndex,
});
setIsOpen(res.status);
},🤖 Prompt for AI Agents
In @web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx around lines 37 -
44, The call to castCommit passes currentRoundIndex which can be undefined
(derived from disputeData?.dispute?.currentRoundIndex); guard it before calling
castCommit by returning early or showing an error/loading state when
currentRoundIndex is undefined, e.g., check if currentRoundIndex == null and
abort the commit flow (do not call castCommit) or provide a valid default, then
only call castCommit with the validated roundIndex and setIsOpen(res.status) on
success.
| const res = await castCommit({ | ||
| type: isGated ? DisputeKits.GatedShutter : DisputeKits.Shutter, | ||
| disputeId: parsedDisputeID, | ||
| choice, | ||
| voteIds: parsedVoteIDs, | ||
| roundIndex: currentRoundIndex, | ||
| justification, | ||
| decryptionDelay, | ||
| }); | ||
| setIsOpen(res.status); |
There was a problem hiding this comment.
Potential undefined value for roundIndex.
Same issue as Classic/Commit.tsx - currentRoundIndex could be undefined. Add a guard before calling castCommit.
Suggested guard
const handleCommit = useCallback(
async (choice: bigint) => {
const decryptionDelay = (countdownToVotingPeriod ?? 0) + 300;
+ if (currentRoundIndex === undefined) {
+ console.error("Round index not available");
+ return;
+ }
const res = await castCommit({
type: isGated ? DisputeKits.GatedShutter : DisputeKits.Shutter,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = await castCommit({ | |
| type: isGated ? DisputeKits.GatedShutter : DisputeKits.Shutter, | |
| disputeId: parsedDisputeID, | |
| choice, | |
| voteIds: parsedVoteIDs, | |
| roundIndex: currentRoundIndex, | |
| justification, | |
| decryptionDelay, | |
| }); | |
| setIsOpen(res.status); | |
| if (currentRoundIndex === undefined) { | |
| console.error("Round index not available"); | |
| return; | |
| } | |
| const res = await castCommit({ | |
| type: isGated ? DisputeKits.GatedShutter : DisputeKits.Shutter, | |
| disputeId: parsedDisputeID, | |
| choice, | |
| voteIds: parsedVoteIDs, | |
| roundIndex: currentRoundIndex, | |
| justification, | |
| decryptionDelay, | |
| }); | |
| setIsOpen(res.status); |
🤖 Prompt for AI Agents
In @web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx around lines 55 -
64, The call to castCommit uses currentRoundIndex which can be undefined; before
invoking castCommit in Commit.tsx check that currentRoundIndex is a valid number
(e.g., !== undefined/null) and bail out or show an error if not set so you never
pass undefined as roundIndex; update the code path that currently calls
castCommit({ ..., roundIndex: currentRoundIndex, ... }) to guard on
currentRoundIndex and only call castCommit when it is defined, referencing the
same symbols (castCommit, currentRoundIndex,
DisputeKits.GatedShutter/DisputeKits.Shutter, setIsOpen) so the UI handles the
missing round index safely.
PR-Codex overview
This PR focuses on enhancing the functionality and structure of the commit process in the application, particularly for various dispute kits. It introduces new builders, improves hashing methods, and adjusts configurations for local development.
Detailed summary
CommitContextinterface inweb/src/actions/commit/context.ts.shutterCommitBuilder,gatedCommitBuilder, andclassicCommitBuilder.web/src/utils/crypto/hashJustification.tsandweb/src/utils/crypto/hashVote.ts.generateSaltmethod inweb/src/utils/crypto/generateSalt.ts.contracts/deploy/utils/index.ts.web/src/actions/commit/execute.tsto utilize new builders.web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsxandweb/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx.web/.env.local.publicfor local deployment.web/src/hooks/queries/useDrawQuery.ts.Summary by CodeRabbit
Documentation
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.